-
Notifications
You must be signed in to change notification settings - Fork 452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
r/compute_cluster: New resource #487
Conversation
This commit adds pretty much most of the workflow code for Create, in addition to other functions needed for read and update. More helpers will probably be added as work continues on the resource.
This commit includes the resource flatteners for the entire resource.
Finished rest of CRUD and import, resource is now ready to have acceptance tests written for it. Commit also contains some small modifications to the vsphere_datastore_cluster resource as I noticed some error messages were off as I was copying the code (a lot of the code was drawn from this resource as a template).
Tests are failing right now, but this is the main scaffolding. Currently working on the basic test right now, working through config spec issues.
Had to fix a couple of configuration issues (one incorrect default, add an attribute skip to prevent a spurious diff), and we were missing host add/removal on update. Both of these have been fixed, and now the basic example passes.
Added/updated/validated the DRS/HA test as functional, Ensuring basic DRS and HA works. Also added a force_evacuate_on_destroy flag - this allows us to simplify testing by having the resource auto-evacuate the cluster on destroy. Users may also want this option as well but I have disabled it by default as auto-evacuation is by nature fragile, as its smooth operation pretty much depends on a cluster devoid of VMs so that no HA constraints are hit during the removal of hosts.
All test have now been updated and vetted. Small updates to things (namely folder helpers) to accommodate. Moving on to documentation.
Forgot to add this one - does a test where we create a VM in the cluster. This one depends on the test environment having the same setup as the live cluster normally used for VM testing. This is the case in our own lab and was how VSPHERE_ESXI_HOST5-7 were intended to be used.
Documentation is now complete, in addition a few updates to attributes and descriptions in the schema.
Added a function to set defaults for all attributes that are not covered in read, except for datacenter_id which we do in a separate read function to make sure that is always set, even on import. This allows a clean import test without requiring any variables be entered into ImportStateVerify.
@@ -220,6 +224,18 @@ func DatastoreFolderFromObject(client *govmomi.Client, obj interface{}, relative | |||
return validateDatastoreFolder(folder) | |||
} | |||
|
|||
// HostFolderFromObject returns an *object.Folder from a given object, and | |||
// relative host folder path. If no such folder is found, of if it is not a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"...found, or if it is not a"
`120` (2 minutes). | ||
* `ha_vm_maximum_resets` - (Optional) The maximum number of resets that HA will | ||
perform to a virtual machine when responding to a failure event. Default: `3` | ||
* `ha_vm_maximum_failure_window` - (Optional) The length of the reset window in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this in minutes or seconds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's seconds, contrary to this diff here it's been fixed (just added on another line).
whether or not specific VM operations are permitted in the cluster in order to | ||
protect the reliability of the cluster. Based on the constraints defined in | ||
these settings, operations such as power on or migration operations may be | ||
blocked. To ensure that enough capacity remains to react to host failures. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"blocked to ensure..."
A large number of settings in the `vsphere_compute_cluster` resource require a | ||
specific version of vSphere to function. Rather than include warnings at every | ||
setting or section, these settings are documented below. Note that this list | ||
are for cluster-specific attributes only and does not include [`tags`](#tags), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"is for..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just the one possible issue and a few grammatical changes.
DrsConfig: expandClusterDrsConfigInfo(d), | ||
} | ||
|
||
if version.Newer(viapi.VSphereVersion{Product: version.Product, Major: 6, Minor: 5}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all doing the same version check. It looks like they either need to be checking different versions or can be combined into one conditional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are now consolidated in ace5a9c.
return err | ||
} | ||
|
||
if version.Newer(viapi.VSphereVersion{Product: version.Product, Major: 6, Minor: 5}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value is in seconds, which was left out of the ha_vm_maximum_failure_window value.
For the version requirements section this time.
These were doing all the same version check, so just group them up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks @bill-rich! |
This adds the
vsphere_compute_cluster
resource, which allows one to manage compute clusters within Terraform.The current supported workflow requires that the hosts already exist in vSphere. We have opted to use this method because it allows for a unified workflow for getting hosts into (requiring that they can be referenced as a
HostSystem
object already) and out of (putting a host into maintenance mode, evacuating it, and moving it to a neutral spot in inventory) a cluster, versus having to support this plus additional methods, like allowing hosts to be added withAddHost_Task
(which has its own implications, such as passwords needing to be added in state).Eventually, the latter workflow may be relegated to a
vsphere_host
resource, which we could use to handle the lifecycle of a standalone host, which could then be used as a dependency tovsphere_cluster
. For now, this allows people to add their hosts with a tool like govc, and then get their data as inputs for the cluster with thevsphere_host
data source.